Skip to content

security: harden user-input attack surface (#50-#59)#41

Merged
jkyberneees merged 20 commits into
mainfrom
fix/sec-findings-remaining
Jun 17, 2026
Merged

security: harden user-input attack surface (#50-#59)#41
jkyberneees merged 20 commits into
mainfrom
fix/sec-findings-remaining

Conversation

@jkyberneees

Copy link
Copy Markdown
Contributor

This PR addresses the remaining security findings from the holistic review of user-input vectors (Telegram, CLI, Web UI).

Fixes included

#50-#53 (earlier commits on this branch)

  • Escape Telegram MarkdownV2 text before sending via send_message
  • Cap /api/resources?limit= to 100 in handler and registry
  • Add global WebSocket connection semaphore (20) and per-IP upgrade rate limiting
  • Cap sub-agent progress stream at 100k lines / 100 MiB; cancel child on overflow

#54-#59 (this session)

  • Scope odek subagent --task deletion to odek temp files only
  • Create fsatomic.WriteFile temp files with exact perms via O_EXCL + random suffix
  • Sanitize resource search query: 256-byte cap + glob metachar escaping
  • Create schedule directory with 0700 permissions
  • Read config via single Open + LimitReader to remove size-check TOCTOU
  • Document advisory flock semantics in package doc

Verification

  • go build ./...
  • go vet ./...
  • go test ./... -count=1
  • `go test -race ./cmd/odek/ ./internal/resource/ ./internal/fsatomic/ ./internal/schedule/ ./internal/config/ ./internal/flock/ -count=1"

Docs

  • docs/SECURITY.md updated with defenses 43-48 and attack-vector rows

Closes remaining findings from the security review.

Replace the package-global ingestRecorder callback with a context.Context
value carried through the agent loop. This removes the race where
concurrent WebSocket sessions could overwrite each other's audit
attribution.

Key changes:
- internal/loop: add WithIngestRecorder / IngestRecorderFrom helpers.
- cmd/odek/untrusted.go: wrapUntrusted now reads the recorder from ctx;
  remove setIngestRecorder globals.
- cmd/odek tools: embed ctxTool and pass toolCtx() to wrapUntrusted.
- cmd/odek serve/main: inject per-session/per-prompt recorder into ctx.
- odek.go: toolAdapter forwards SetContext to odek.Tool implementations.
- Tests: add unit, integration, and concurrency regression tests.

Fixes finding #20 from sec_findings.md.
Replace the global currentPromptCancel atomic.Value with a mutex-protected
map keyed by session ID. POST /api/cancel now requires a session_id query
parameter, so one WebSocket connection cannot cancel another connection's
running prompt.

Changes:
- cmd/odek/serve.go: add promptCancels map + register/unregister/cancel helpers;
  handlePrompt registers/unregisters the cancel func for the session it runs;
  handleCancel requires session_id.
- cmd/odek/ui/app.js: include sessionId in cancel fetch URL.
- cmd/odek/serve_test.go: update existing cancel tests to pass session_id;
  add TestServe_Cancel_MissingSessionIDReturns400 and
  TestServe_Cancel_CannotCrossSessions regression test.

Fixes finding #19 from sec_findings.md.
Store.Save already redacted Message.Content and ReasoningContent, but the
session Task field (the first user prompt / session title) was written to
disk verbatim. Secrets in the first prompt leaked into the session file and
index, and were returned by the session APIs.

Changes:
- internal/session/session.go: redact sess.Task in saveLocked before
  marshalling and updating the index.
- internal/session/session_test.go: add TestStore_SaveRedactsTask covering
  in-memory, on-disk, and index redaction of the Task field.

Fixes finding #21 from sec_findings.md.
Replace filepath.Glob in glob and search_files(target=files) with a
workspace-confined walk helper. filepath.Glob follows symlinked directory
components and resolves .. segments, allowing patterns like link/*.txt or
../.ssh/id_* to enumerate files outside the working directory.

Changes:
- cmd/odek/file_tool.go: add confinedGlob helper using filepath.WalkDir,
  skipping all symlinks, rejecting .. and absolute patterns, and verifying
  every match stays inside the resolved root. Refactor globTool.Call and
  searchFilesTool.searchFiles to use it.
- cmd/odek/security_vulnerabilities_test.go: add
  TestGlob_DotDotPatternRejected, TestSearchFiles_DotDotPatternRejected, and
  TestGlob_AbsolutePatternRejected.

Fixes finding #22 from sec_findings.md.
buildSubagentRequest previously wrapped untrusted parent input in constant
<untrusted_input> tags with no per-call nonce and no neutralisation of the
marker string inside the body. A crafted </untrusted_input> in the goal or
context could close the fence early and inject instructions.

Changes:
- cmd/odek/subagent.go: add wrapUntrustedSubagentInput and
  neutraliseSubagentInputLiterals helpers. Generate a random nonce per
  request, emit <untrusted_input_<nonce>> tags, and replace literal
  occurrences of untrusted_input with a look-alike so injected close tags
  cannot pair with the wrapper.
- cmd/odek/subagent_prompt_isolation_test.go: update
  TestSubagentRequest_UntrustedIsFenced for nonce'd tags; add
  TestSubagentRequest_UntrustedNeutralisesCloseTag regression test.

Fixes finding #24 from sec_findings.md.
InjectFiles used os.Stat, which follows symlinks, and only verified that
the symlink path itself was under cwd. A symlink committed inside the
project pointing at an arbitrary host file (e.g. leak -> /etc/shadow)
would have its target copied into the container.

Changes:
- internal/sandbox/sandbox.go: use os.Lstat in InjectFiles; reject any ctx
  file that is a symlink. Resolve cwd to an absolute path and use
  isPathUnder for the relative-path check.
- internal/sandbox/sandbox_test.go: add TestInjectFiles_SkipsSymlink.

Fixes finding #25 from sec_findings.md.
BuildRunArgs only rewrote 'host' to 'none', but other Docker network modes
such as 'container:<name>' were passed through unchanged. That allowed the
sandbox to share another container's network namespace and bypass intended
network isolation.

Changes:
- internal/sandbox/sandbox.go: enforce an allowlist of 'none' and 'bridge'
  for --sandbox-network; reject any other value (including 'container:...')
  by forcing it to 'none' with a warning. Empty Network defaults to
  'bridge'.
- internal/sandbox/sandbox_test.go: add tests for container:, bridge,
  empty, and host network modes.

Fixes finding #26 from sec_findings.md.
spawnChild copied os.Environ() and re-injected ODEK_API_KEY,
DEEPSEEK_API_KEY, and OPENAI_API_KEY, leaving the key visible in the child
process environment (/proc/<pid>/environ) and crash dumps.

Changes:
- cmd/odek/telegram.go: reuse the existing FD-based key handoff helpers
  (writeKeyToUnlinkedFile / readKeyFromInheritedFD). telegramCmd reads the
  key from  if present; spawnChild writes the resolved key
  to an unlinked tempfile and passes the FD to the child via the safe
  ODEK_API_KEY_FD signal env var instead of the key itself.
- cmd/odek/telegram_test.go: replace env-injection test with a ProcAttr
  interception test that verifies the key is passed via FD 3 and absent
  from the child env.

Fixes finding #28 from sec_findings.md.
sanitizeDocName only stripped directory components and rejected empty/.
/.. names. It allowed hidden files, arbitrary characters, and very long
filenames, so an attacker could send a document named '.bashrc' or an
overlong filename that would be saved under ~/.odek/media/.

Changes:
- internal/telegram/download.go: reject names starting with '.', restrict
  characters to [A-Za-z0-9._-] (replacing others with '_'), enforce a
  maxDocNameLen of 200 while preserving the extension, and factor out
  fallbackDocName.
- internal/telegram/download_test.go: add test cases for hidden files,
  unsafe characters, Unicode names, and overlong names.

Fixes finding #30 from sec_findings.md.
- AutoSaveSuggestions now skips suggestions with untrusted provenance unless allowUntrusted is true.

- RunAutoSaveLoop declines tainted skills by default and logs them.

- promoteSkill refuses to clear NeedsReview on tainted skills without --force.

- CLI help, SECURITY.md, LEARNING.md, README.md, and AGENTS.md updated.

- Added/updated tests for promote refusal/force and auto-save decline/allow paths.
…_execution

- Add embeddedShellInterpreters map for awk/gawk/mawk/nawk, ed/ex, vi/vim/nvim/view, emacs/emacsclient.

- Classify their non-version/help invocations as code_execution.

- Detect sed 'e' command, -f/--file, and s///e flag as code_execution.

- Remove awk from writePrefixes; add embeddedShellInterpreters to isKnownCommandName.

- Add regression tests for awk, sed, vim, find -exec bypasses.

- Update SECURITY.md and AGENTS.md.
- Replace unbounded bufio.Reader.ReadString with bufio.Scanner limited to 10 MiB per line.

- On oversized line, report error to pending callers and close the connection.

- Add TestReadLoop_OversizedResponse regression test.
- Validate MCP tool names in internal/mcpclient/client.go Discover:
  ASCII letters/digits/_/-, non-empty, <=64 chars.
- Reject raw MCP tool names that shadow odek built-ins in cmd/odek/main.go
  loadMCPTools, even though prefixed names are normally used.
- Add per-tool approval for project-level MCP servers in
  cmd/odek/mcp_approval.go, persisted in ~/.odek/mcp_tool_approvals.json.
- Reuse ODEK_APPROVE_MCP env var for both server and tool approvals.
- Add unit tests and E2E coverage for name validation, shadowing, and
  approval gating.
- Update docs/SECURITY.md, AGENTS.md, and docs/MCP.md.
…, #40)

- Add 10 MiB size checks to count_lines, checksum, head_tail, and
  word_count before scanning/hashing, matching other perf tools.
- Add maxInlineContentBytes (10 MiB) and enforce it on base64 inline
  string/content and tr inline content arguments.
- Add tests for each new size-cap rejection path.
- Update docs/SECURITY.md and AGENTS.md.
…tent

- Add exact-size boundary tests for count_lines, checksum, head_tail,
  word_count, base64 inline content, and tr inline content.
- Add tail-mode and decode-string oversized rejection tests for head_tail
  and base64 to cover both branches.
- Add CHEATSHEET note listing the perf tools with 10 MiB file/inline caps.
…ters (#41-#43)

- internal/schedule/store.go: fileLock now returns an error instead of a
  silent no-op fallback; all mutating callers (Add, Put, Remove, SetEnabled,
  SaveState) abort when the flock cannot be acquired.
- internal/schedule/store.go: readJSON now Stat()s schedule/state files and
  rejects anything larger than 10 MiB before reading.
- internal/loop/loop.go: tool-result delimiter now embeds a per-call random
  hex nonce in both the opening and closing lines, preventing a tool or MCP
  server from forging the closing delimiter.
- Add/update tests for lock failure, oversized schedule file, exact-size
  boundary, and nonce uniqueness.
- Update docs/SECURITY.md and AGENTS.md.
…dings (#44-#48)

- parallel_shell: bind commands to agent context via CommandContext, run
  each in its own process group, kill the group on cancellation/timeout,
  and cap per-command timeouts at 30 minutes.
- batch_patch: add trustedClasses field and pass it to CheckOperation for
  consistent approval behavior with write_file/patch.
- browser: wrap clickableRef.URL as untrusted; keep rawURL for internal
  click resolution.
- telegram: enforce MaxMsgLength in UTF-16 code units instead of bytes.
- telegram: write restart.json with 0600 instead of 0644.
- Add regression tests for all five fixes.
- Update docs/SECURITY.md and AGENTS.md.
…urrent

- Add TestParallelShell_ContextCancel_Explicit to cover the
  context.Canceled branch in runOne.
- Add TestBrowser_Click_ButtonAcknowledges and
  TestBrowser_Click_InputSubmitAcknowledges to cover button/submit click
  paths and increase browser_tool.go coverage.
- Update docs/CHEATSHEET.md: parallel_shell process-group kill, browser
  URL wrapping, Telegram flock/0600 restart marker/UTF-16 length limits.
…ap, WS connection limits, sub-agent progress bounds)

- #50: Escape agent-generated text in send_message before Telegram MarkdownV2 send
- #51: Cap /api/resources limit to 100 in handler and Registry.Search
- #52: Enforce max 20 concurrent WebSocket connections + per-IP upgrade rate limit
- #53: Cap sub-agent progress stream at 100k lines / 100 MiB and cancel child on overflow

Tests and docs (SECURITY.md, CHEATSHEET.md) updated.
…, schedule perms, config TOCTOU, flock docs)

- Scope sub-agent --task file deletion to odek temp files only
- Create fsatomic temp files with exact perms via O_EXCL + random suffix
- Sanitize resource search query: 256-byte cap + glob metachar escaping
- Create schedule directory with 0700 permissions
- Read config via single Open+LimitReader to remove size-check TOCTOU
- Document advisory flock semantics in package doc
- Update SECURITY.md with defenses 43-48 and attack-vector rows
@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
odek c223e5d Commit Preview URL

Branch Preview URL
Jun 17 2026, 10:59 AM

@jkyberneees jkyberneees merged commit d9906d9 into main Jun 17, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant